Skip to content

Conversation

@laurit
Copy link
Contributor

@laurit laurit commented Nov 10, 2025

Instead of bumping class version to 1.7 so invokedynamic instruction could be used this PR introduces a more conservative approach. If class version supports invokedynamic we'll add the invokedynamic instruction as we do now. If the class version does not support invokedynamic we generate an helper class that contains the invokedynamic instruction in a static method and invoke that method via invokestatic. This way we don't have to worry about jvm class file parsing being more strict for newer class versions or building the stack map etc.

@laurit laurit marked this pull request as ready for review November 10, 2025 15:09
@laurit laurit requested a review from a team as a code owner November 10, 2025 15:09
Comment on lines 149 to 150
boolean allowClassVersionChange =
config.getBoolean("otel.javaagent.experimental.allow-class-version-change", false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now left a flag for switching back to changing the class version, if there is no interest in keeping it could remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support fewer options 😄

@SylvainJuge
Copy link
Contributor

This is a very clever idea, simpler and less risky than trying to update bytecode version on the fly. Do we have any case where injecting helper classes is not possible, for example due to a non-cooperative classloader implementation ? If no, this looks definitely safe to use this strategy by default.

This way we don't have to worry about jvm class file parsing being more strict for newer class versions or building the stack map etc.

I know recomputing the stack map can be costly, but I have to admit that I never actually measured it nor seen any major impact in the application startup overhead. Have you seen any significant difference with this change ?

Also, do you have any known example of strict class parsing causing an issue ? This is more curiosity here, but is that something we expect to happen more with current and future JVM versions ?

@laurit
Copy link
Contributor Author

laurit commented Nov 13, 2025

Do we have any case where injecting helper classes is not possible, for example due to a non-cooperative classloader implementation

We are using the same approach as we currently use for helper class injection. We insert loading the helper classes into loadClass in ClassLoader and its subclasses. It could fail in case there is a class loader that doesn't get instrumented, because it is excluded from instrumentation, and does not delegate to ClassLoader.loadClass.

I know recomputing the stack map can be costly, but I have to admit that I never actually measured it nor seen any major impact in the application startup overhead.

The question is usually not about the cost but more about whether it can be reconstructed at all. The complicated part is that given types A and B you need to be able to find a common super type. For that you need to be able to find the bytes for these classes and their supertypes. As far as I recall byte buddy type pool relies on ClassLoader resource lookup methods for this. Unfortunately this doesn't always work for example when you have a type A with super type S, defined in class loader CA, that is used in type B defined, in class loader CB it is possible that class loader CA sees and is able to find the type S but class loader CB is not. You can see this happening in the webshpere smoke tests https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/19320896253/job/55262002583 search for Cannot resolve type description for. It could also be that some types are missing at runtime. The funny thing is that with javac generated bytecode you could get away by just saying that the common super type is Object due to the way javac inserts casts, but that won't work with bytecode from eclipse compiler. Anyway when it doesn't work correctly it is going to be a hassle so imo best to avoid it.

Also, do you have any known example of strict class parsing causing an issue ?

Have a look at https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/classFileParser.cpp and search for _major_version Most of the checks are for features introduced in newer version but there are some like https://github.com/openjdk/jdk/blob/6b6fdf1d9222eb03cd013cbe792fa77fd78c1acb/src/hotspot/share/classfile/classFileParser.cpp#L2168 that relax checks for backwards compatibility. I have previously worked on an agent that changed class version and instrumented all the classes and we did run into these and had to work around them in our class transformations. We definitely had a transform that made <clinit> static and also a workaround for https://github.com/openjdk/jdk/blob/6b6fdf1d9222eb03cd013cbe792fa77fd78c1acb/src/hotspot/share/classfile/classFileParser.cpp#L3016 Since the otel agent doesn't transform all the classes the chances of hitting these issues is arguably much smaller. Also many of them require classes that were compiled with prehistoric versions which hopefully will be harder to encounter as time goes by. Again this is something that will not affect the vast majority of the users but when it happens it will be annoying.

Comment on lines 149 to 150
boolean allowClassVersionChange =
config.getBoolean("otel.javaagent.experimental.allow-class-version-change", false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support fewer options 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants